-
Notifications
You must be signed in to change notification settings - Fork 80
C++ support for EUMM #45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add option XSTARGET_EXT to WriteMakefile (it is ".c" by default). Generated Makefile under ExtUtils::MM_Unix now contains variable XSTARGET_EXT and uses it as suffix in files produced by xsubpp. Also add right files in C_FILES list.
Plus try fix all other xs_c,xs_o methods (only in VMS really).
This commit skips t/xscpp.t tests if user has no C++ compiler.
|
Impressive work, and tests! To move this forward it needs two things. First is documentation. The MakeMaker XS documentation is a mess, if it doesn't slot in there maybe add an entry to ExtUtils::MakeMaker::FAQ about how to incorporate C++ code into the project. Second is having somebody who understands XS and C++ look this over. Your changes look small and don't rewrite large swatches of code, which is good from my perspective, which is lacking understanding of XS and C++. Lemme see if I can scare up a reviewer. |
|
It passes my average C++ knowledge, and I'm going to try it against the two XS C++ projects I have right now. |
|
I get a WARNING: "XSTARGET_EXT" is not a known parameter, but the resulting Makefile does include the XSTARGET_EXT variable. And then "cc" correctly compiles the .cpp file as C++ (which means I can remove my "CC => 'g++'" option in WriteMakefile()). So, it works for me. A warning: If you're going to start using this in an existing project, be sure to make clean before you add XSTARGET_EXT. If you don't remove the old .c files, it will try to compile them (which will probably fail). I spent 10 minutes thinking this didn't work because those old .c files were hanging around. |
|
@preaction Thanks for having a look. I think I found where the warning is coming from. Good note about cleaning up the old .c files. That should go into the FAQ about this when it gets written. |
|
Thanks for review! I'am just fix warning as suggested + fix formatting... I will try write small intro, but almost certainly it will need help to tune wording, grammar etc... (because my english is bad ;-) |
…so improve formatting for XSTARGET_EXT option description
|
I'am just add some docs about XSTARGET_EXT and using it to build XS with c++ compiler. I will be very grateful for pointing me in the syntactic and stylistic errors. |
|
So... can I do something to help merge this request now? Because after that, I can continue work to improve C++ support:
|
|
I think it's a useful hint for compilers that use extensions for heuristics, but there will be compilers that don't support that. That portability limitation should be explained. Secondly, it seems a «make test» invocation is missing in t/xscpp.t (which is actually a bug it inherited from t/xs.t). When fixing that, it fails just as I had expected: Thing is, it doesn't solves the issue of linking C++. You can't generally expect a C compiler to link a C++ object correctly. |
|
Firstly, thanks for review! |
Use quiet mode of the ExtUtils::CBuilder instead of magic with STDOUT/STDERR
Remove preprocessor tests for __cplusplus, we already know that this is C++
1. Make xs.t and xscpp.t really run "make test" 2. Fix XSCPP module Makefile.PL to add libstdc++ to link
|
Has this work stalled? it looks like it was almost complete. |
|
Yes, in a sence.
So what solution acceptable by community? |
|
Surely the right way is:
|
|
@vovkasm, feel like the tiny tweak this will take? Or want a hand? |
|
By the way, if you did decide to go for your approach 2, this already does detection of compilers etc, so ought to make life easy: https://github.com/daoswald/Inline-CPP/blob/master/Makefile.PL Indeed, if EUMM gains the capability to do this itself, it will simplify I::CPP's Makefile.PL. Including @daoswald in case he's interested. |
|
I am interested. In addition to Inline::CPP's (ugly but necessarily so) Makefile.PL, you can also look at ExtUtils::CppGuess for work in the area of proper detection of compiler name and libraries for linking purposes. In fact, there's a test included in Inline::CPP's suite that provides a comparison of my Makefile.PL selection versus the ExtUtils::CppGuess selection so that when I look at test FAILs I can see if one might do a better job than the other. There is still much work to do in this area; we still seem to not be 100%. smueller might have additional input (I believe he authored ExtUtils::CppGuess). |
|
Is it crazy to think that we can unify I::CPP's Makefile.PL, @tsee's https://github.com/tsee/extutils-cppguess, and this, and just use (probably) E::CppG? |
* commit '3f38c3a0136464f543c714a10d3fba557afc0d2e': Only bundle JSON::PP::Compat5006 if $] < 5.008
|
Am 20.08.2014 um 21:47 schrieb Michael G. Schwern [email protected]:
You can study some c++ code by reviewing my c++ projects on GitHub. When you (or someone else) has serious interest - I can develop the
Ask for something more valuable.
There is a Config::AutoConf already - no need to use native (non-portable)
Not from me, however. Please offer a bit more than merging in case of What I can offer would be 2 XS implementations for Hash::Merge - a C But the EU::MM side must be done by someone else. CheersJens Rehsack |
|
I have a much better module proposal for testing: Lib::Log4cplus (perl-wrap Logging Framework for C++) and link using c++ in Unix::Statgrab when log4cplus is used. Anyway - I would need help in doing the EU::MM integration - the right flags and cxx switches will be delivered by Config::AutoConf. |
|
The vital question is: which EUMM params would you need to pass, that you can't currently? I predict |
|
Am 22.08.2014 um 21:35 schrieb mohawk2 [email protected]:
I have no clue what XSTARGET_EXT is for and I cannot guess it. I need to pass generally
There should be one difference kept in mind - writing an XS module using C++ but native (C-)API can do using xsubpp (XSC) but needs CXX to compile and CXXLD to link while C++ XS with c++ types (STL ones?) one need xspp (XSXX) and similar for f95, cob ... The linker to be used is usually: ccld < cobld < fnnld < cxxld C++ is the most complex environment, a suitable objdump utility is required when mixing more than 2 languages (to get the required runtime libs). Best regardsJens Rehsack |
|
No reply within a week but actively discussion on other topics I rate this topic evolves not Perlish enough? |
|
@rehsack, maybe you didn't mean it that way, but that looked like you sniping. I'd prefer that instead you be more proactive in answering questions put to you on various topics, such as #75 (comment). @vovkasm, rather than merge across other more recent changes, which will cause big problems to merge this PR, could you back out that merge, then rebase your master against current PTG master and then push -f it? If you need help with the fairly hairy git commands involved, I can assist. |
|
@vovkasm, I strongly suggest you bring your copy of EUMM up to date first, so you're not aiming at a moving target. If you're happy to take this forward by yourself, please do. If I can help (possibly by seeking wisdom from @daoswald), let me know. @rehsack, by the way - as part of my "perl in place-with-space" magnum opus, I'm currently fixing up dmake on https://github.com/mohawk2/dmake - non-Perlish enough for you? ;-) |
|
From @daoswald:
The module above is on CPAN and uses Inline::CPP. It passes tests on 226 systems, and only has the death penalty on one. @vovkasm, feel like extracting ideas from this? Or are you busy on other stuff? |
That's 226 systems, but only two compilers: gcc and clang. Not even msvc appears to be represented. |
|
I am a Visual C user. I'll ignore the lack of VC compatbility, since it is trivial https://metacpan.org/source/BJOERN/Graph-NewmanGirvan-0.3/Makefile.PL#L32 . I disagree with XSTARGET_EXT. XSTARGET_EXT says a module must be entirely C++ or C, not a mix of both. The better solution would be either .xs.cpp and then a .cpp.obj rule, and use an explicit dependency name (Foo.obj) to trigger the .xs->.cpp->.obj building, or a array ref or hash ref passed to %options should say which xs files are C++, and everything left out is C. |
|
@bulk88, hmm... interesting
Did you mean that if XSTARGET_EXT is ‘.cpp’ then all *.xs files in dist will be translated to *.cpp? And no, if you mean that XSTARGET_EXT says a module must be entirely C++. XSTARGET_EXT simply says what suffix *.xs files should have after translating with xsubpp, you of course may have mix of *.cpp and *.c files in dist and they will be processed as expected. So you may write module in C++ and embed C-only library. But I don’t against the idea of using ’special’ suffix for XS files to mark they as ‘XS with C++’... In implementation there are some problems (that I can see):
|
|
Open the question up to other participants: does |
|
@mohawk2 - a bit of sniping whom? I just recognized a stalling, but important issue discussion. My point is: ExtUtills::MakeMaker is the Perl equivalent to AutoMake - both are makefile generators. I think, before extensions a la XSTARGET_EXT can be reasonable implemented, kind of refactoring in the EU::MM kernel are necessary to allow building a "used tool" fundament. When I would do a Lib::Log4CPlus - I would run into similart issues @vovkasm describes, because I would create neat XS and call the C++ API in it - no export and mungle of C++ types to Perl. |
Yes.
Yes. Suffix rules is how makefiles are supposed to work in spirit. So .xsp is the easiest way. Just make your file named .xsp and EUMM does all the rest.
That sounds like another bug. I am not going to test or IDK the status of cmd line env vars affecting the make driver building process, or affecting the makefile file's macro contents. |
|
There is more precedent for XSpp
|
|
XSpp is not a precedent for anything. |
|
Am 06.09.2014 um 11:51 schrieb bulk88 [email protected]:
It's not another bug - and it's not a question of cmd line env vars - it's a fundamental concept issue. Please work a little bit in cross-compiling environments, pre-building environments for large data centers having lots of platforms and bulk-build to make a difference on environment variables and configure stage scripts analyzing capabilities of target platforms. Best regardsJens Rehsack |
|
I request everyone stop posting on this thread for 48 hours. Take a step back, it will wait. Enjoy your weekend. |
|
I was brought in as a 3rd party to look at this issue as well. I see the fundamental problem being the following: The patch does add working support for C++ with a GNU-compatible toolchain in mind (this means g++ and clang++). The same problem is also present with ExtUtils::XSpp, by which this patch seems to be inspired. The fundamental question to the EUMM maintainers and power-users in this case is - is the worse-is-better approach of The positions of the main participants in this ticket are already clear, I would like to solicit further input specifically from @dagolden, @rjbs, @Tux, @andk and @timbunce. |
|
riba asked me to reply by mail. I has a long mail typed when the power dropped. Content lost. <[Tux]> I love EU::MM for its simplicity towards the developer |
|
I have only skimmed the thread, so I'm just commenting on design philosophy issues that @ribasushi raised. When dismissing "worse is better", I think it's also important to remember that "the perfect should not be the enemy of the good". If we can deliver useful features for 99% of end-users, I don't mind throwing an "unsupported" error for the remaining 1%. My minimum standard for inclusion would be that this should support the default compilers for the vast majority of mainstream Unix-like systems (Linux, (Free|Net|Open)BSD, Darwin, Solaris, Cygwin) and the major compilers for MSWin32 (MSVC and gcc). If support is lacking for VMS or Android or other specialized systems, I think that's OK. |
|
With #168 I've just been making support for multiple *.xs files, which is conceptually related to supporting e.g. 1 C XS, and 1 C++ XS file. I am increasingly convinced the way forward for C++ XS is to have a different file-extension, almost certainly |
|
Nice test cases against which I can test, and from whose current build system I can steal ideas: Modules that do XS++ support:
Other modules that use XSpp: https://metacpan.org/requires/distribution/ExtUtils-XSpp?sort=[[2,1]] |
|
As #195 provides better solution, this pull request can be freely closed. |
This patch adds C++ support for EUMM. It introduse parameter (and Makefile macro) with name
XSTARGET_EXT- it is suffix for files prodused from .xs (.c by default).This allows modules written in C++ does not use various hacks in order to run the C++ compiler for ".с" files.
I personally test this change with:
Only warning. I blindly made changes in
ExtUtils::MM_VMS(because I personally can't test for this platform)